-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make transport-cc feedback work similarly to libwebrtc #1088
Conversation
Thanks, will check next week. |
@penguinol, do you think you can write few tests confirming this fix? The scenarios to test are clear. |
Co-authored-by: Iñaki Baz Castillo <ibc@aliax.net>
…soup into fix_tcc_feedback
The input of |
I'll come back to this PR this week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@penguinol, any change we can have a test for this change?
Co-authored-by: José Luis Millán <jmillan@aliax.net>
I'll try to add some tests next week. |
Modify nowMs as a parameter for testing purposes.
…soup into fix_tcc_feedback
fix situation of [1 2 4 5] [3 6 7]
@jmillan i've add some tests |
Co-authored-by: Iñaki Baz Castillo <ibc@aliax.net>
use std::addressof
In addition to requested/proposed changes, please merge v3 branch into yours and add a |
This random failure in a CI test was fixed in v3 branch recently, please merge v3 branch into yours. https://github.com/versatica/mediasoup/actions/runs/7710876267/job/21015131079?pr=1088#step:7:57 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor cosmetic changes/fixes requested.
Other than those I think we are good.
Co-authored-by: Iñaki Baz Castillo <ibc@aliax.net>
Co-authored-by: José Luis Millán <jmillan@aliax.net> Co-authored-by: Iñaki Baz Castillo <ibc@aliax.net>
Co-authored-by: Iñaki Baz Castillo <ibc@aliax.net>
…soup into fix_tcc_feedback
@penguinol I'm pushing pending changes to this branch. |
Merging. Thanks a lot @penguinol!! |
This PR introduces a bug that causes a crash: #1336 |
- Fixes #1336 ### Details - Regression introduced in PR #1088, which has many issues. - Basically there are cases in which `this->transportCcFeedbackPacket` is reset so it's a complete new packet and hence its `baseSet` is `false`. - However in `TransportCongestionControlServer::FillAndSendTransportCcFeedback()` in which we just call `this->transportCcFeedbackPacket.SetBase()` once at the top, but then we enter a loop in which the packet maybe full so it's sent (or other things) and hence we **reset** the packet while in the loop. And then at the top of the loop `this->transportCcFeedbackPacket.AddPacket()` is called so the assertion fails because its `SetBase()` was not called on that fresh packet. - Also add a missing `break` in a `case` block in that loop. - Also set proper packet count in all cases in which we reset `this->transportCcFeedbackPacket`. ### TODO This is not done yet. The issue is mainly identified but I don't know yet how to properly fix it without doing other wrong things. Basically I'm not sure if the whole logic is ok after PR #1088: - As said before we only set the base of the packet once at the beginning of the loop, and such a `SetBase()` is called with `this->transportCcFeedbackWideSeqNumStart` and a computed `timestamp`. - However, within the loop below, the packet may become full and hence `SendTransportCcFeedback()` is called, which updates `++this->transportCcFeedbackPacketCount` and `this->transportCcFeedbackWideSeqNumStart`, and **also** resets the packet (so missing base again). - So in order to fix the crash, we must call `SetBase()` again, but which which values? Because the values given initially at the top of `TransportCongestionControlServer::FillAndSendTransportCcFeedback()` were the value of `this->transportCcFeedbackWideSeqNumStart` at that time and a timestamp. But that `this->transportCcFeedbackWideSeqNumStart` has now changed, so then what? - Basically every time `this->transportCcFeedbackPacket.reset()` is called (and there are various cases in the whole `TransportCongestionControlServer.cpp` file) **we must** call ``SetBase()` on the fresh packet **with** proper values. And which are those proper values if we are within that loop?
fix #1059